Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve startup speed #821

Merged
merged 2 commits into from
Oct 29, 2024
Merged

Conversation

marckhouzam
Copy link
Contributor

@marckhouzam marckhouzam commented Oct 4, 2024

What this PR does / why we need it

This PR avoids writing to the config files and the .data-store.yaml on startup if not necessary.

Writing to the config file to update the server list has a cost for each context. I ended up with 27 contexts configured, which triggered writing to file over 50 times which added 0.65 seconds on my M1 Mac, making the startup jump from around 0.35 seconds to a full second. Having the CLI take a full second to execute any command implies that each call to shell completion takes at least 1 second, which is slow for the user.

Updating the server list does not actually need to be done all the time.

Furthermore, the CLI currently writes to the .data-store.yaml and to the config files on every command when it stores the LastCLIVersionUsed. This file writes are also unnecessary most of the time.

This PR has two commits:

  1. compute a sha of the contexts and another for the servers and store it in the data-store. Then for each command, instead of blindly synching contexts and servers, only do the sync if either sha has changed.
  2. on write the last executed cli version to the data-store if the version is different than what is stored already, and only disable the features.global.context-target-v2 feature if it is currently enabled (to avoid writing to the config files)

This reduces the execution time for commands that don't access the backend; this means that shell completion for commands and flags becomes noticeably more responsive when a user has multiple contexts configured.

Before this PR, running tanzu version with 9 contexts configures takes around 0.5 seconds.
With this PR is becomes closer to 0.3 seconds.

Which issue(s) this PR fixes

Fixes #823

Describe testing done for PR

I use this little utility called entr which will execute a command whenever a specific file is changed.

# Monitor changes to the two config file and the datastore file
$ echo ~/.config/tanzu/config.yaml| entr echo config changed >>out.txt
$ echo ~/.config/tanzu/config-ng.yaml| entr echo config-ng changed >>out.txt
$ echo ~/.config/tanzu/.data-store.yaml | entr echo datastore changed >>out.txt
$ tail -f out.txt

Before the PR, if I run any command such as tanzu -h >/dev/null I see the config files change for every command multiple times (one for each context).

And for commands that trigger the lastTanzuVersion check (most commands), such as tanzu plugin list >/dev/null, I see the datastore file updated and the config files also.

With this PR, running those same commands, I don't see any changes done to the config or datastore files.

I also ran some normal shell completion down the command tree, and the responsiveness is noticeably better.

Release note

Improve CLI execution speed, especially for shell completion responsiveness

Additional information

Special notes for your reviewer

Copy link
Contributor

@vuil vuil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall optimization is quite clean and much appreciated, thanks for looking into this.

I have a very tiny concern if the skipping of the SetContext() has any unintended consequences, and if we want to cover our bases.

pkg/config/context.go Outdated Show resolved Hide resolved
@marckhouzam marckhouzam changed the title Improve startup speed by 65% Improve startup speed Oct 8, 2024
Before this commit, the CLI would always perform a two-way sync of the
Contexts of the config-ng.yaml file and the Servers of the config.yaml
file, even if there was no reason to sync, i.e., if the Contexts
and Servers were already in sync.  This sync was being done for every
single command of the CLI..

This caused a write to the config files for each contexts configured
and one more for the current context.  With many contexts configured,
this unnecessary sync would make the CLI startup noticeably slower.

This commit verifies if the sync is required by computing and storing a
SHA for the "context" section and one for the "server" section, and
checking if the SHAs have changed on each CLI command and if so, only
then doing the sync. Note that Tanzu contexts are kept out of the
SHA computation since they are not synced.

Signed-off-by: Marc Khouzam <[email protected]>
The logic to store the last execute CLI would always write to the
datastore and to the config files, which should be avoided.

This commit first checks if the update is required or not.

Signed-off-by: Marc Khouzam <[email protected]>
@marckhouzam marckhouzam marked this pull request as ready for review October 22, 2024 19:31
@marckhouzam marckhouzam requested a review from a team as a code owner October 22, 2024 19:31
Copy link
Contributor

@anujc25 anujc25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Very nice way to test the change as well. I just have one question.

pkg/config/context.go Show resolved Hide resolved
Copy link
Contributor

@vuil vuil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice approach of comparing the server and context contents!
lgtm!

@marckhouzam marckhouzam merged commit 6cbbf31 into vmware-tanzu:main Oct 29, 2024
7 checks passed
@marckhouzam marckhouzam added this to the v1.6.0 milestone Oct 29, 2024
@marckhouzam marckhouzam deleted the marck/fastStartup branch October 29, 2024 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inefficient startup
3 participants